Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/better find package #1019

Closed
wants to merge 17 commits into from
Closed

Feature/better find package #1019

wants to merge 17 commits into from

Conversation

vguen
Copy link

@vguen vguen commented Jan 18, 2019

This PR is a proposal to fix #732 using find_package in order to have better granularity for handling dependencies (this is inspired by https://gist.github.com/Som1Lse/6458d28c4bb09163a5389f24fec981db).
Thanks to the built-in variable <package_name>_DIR variable we can override the default path for find_package in order to use :

  • some preinstall cmake package
  • custom source repo (either download with BUILD_DEPS=ON or already download and provide by the user through a custom <Package_name>Config.cmake)

Allowing to use find_package in order to resolve out of tree dependencies allow or-tools to integrate nicely with Yocto, which is not the case today as Yocto since we cannot relay on the BUILD_DEPS switch in Yocto (it does not allow to download dependency during cmake configure time).

⚠️ Note that this PR will properly for work preinstall cmake package once abseil/abseil-cpp#182 will be merged

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@vguen
Copy link
Author

vguen commented Jan 29, 2019

@Mizux do you think this PR could be integrate into the code ?

@Mizux
Copy link
Collaborator

Mizux commented Jan 29, 2019

First, thanks for your contribution !

If I understand correctly, you just split the cmake/external/CMakeLists.txt into several DependencyConfig.cmake so for each deps you could choose the system package or ortools will compile it for you.
-> I'm OK with this behaviour...

But in this case the new macro _find_package() and the filename seems strange to me.
I mean you include() them if needed, while for me you should have modified the CMAKE_MODULE_PATH instead so find_package() would find them first instead of the one already installed by your distro package manager...
And call them FindDepency.cmake e.g. FindSWIG.cmake

In Module mode, CMake searches for a file called Find<PackageName>.cmake in the CMAKE_MODULE_PATH followed by the CMake installation. If the file is found, it is read and processed by CMake. It is responsible for finding the package, checking the version, and producing any needed messages. Some find-modules provide limited or no support for versioning; check the module documentation.

ref: https://cmake.org/cmake/help/latest/command/find_package.html#basic-signature-and-module-mode
note: in this case we should have each FindDeps.cmake in a separate directory, so we can selectively add them to CMAKE_MODULE_PATH as needed...

Also, one other side topic: I think I'll have to change the CMake behavior, I would like to install deps first then consume the install, than trying to use it as add_subdirectory(). Currently it's conflict with dependency install rule or you need to hack dependencies to make it work...
see: https://travis-ci.org/google/or-tools/jobs/482350303#L904

Sorry for the delay, I'm still thinking about it and where I would like to go.
Need to play with your branch...

vguen added 2 commits January 29, 2019 16:30
Using CMAKE_MODULE_PATH & Find<Package>.cmake files simplify how we handle the package provide by CMake (ie: ZLIB & Protobuf).
CMakeLists.txt Outdated
endif()
if(NOT abseil_DIR AND BUILD_DEPS)
set(abseil_DIR ${CMAKE_CURRENT_SOURCE_DIR}/cmake/external CACHE PATH "abseil dependency path")
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake/external/abseil)
Copy link
Collaborator

@Mizux Mizux Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abseil-cpp ? (I can also do it in a later commit)
just becaue there is also https://github.com/abseil/abseil-py and maybe we will need it one day too
note: shame on me of not doing it too ;)

@vguen
Copy link
Author

vguen commented Feb 1, 2019

@Mizux I rework the way this branch handle its dependencies in 2a003e0 the branch now donwload, build & install its file in the build directory (no more subproject).

I have however the following compilation error which I do not understand and I would like your help to see what I could have missed.

/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:26:1: error: C++ requires a type specifier for all declarations
DEFINE_int64(jssp_scaling_up_factor, 100000L,
^
/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:26:14: error: use of undeclared identifier 'jssp_scaling_up_factor'
DEFINE_int64(jssp_scaling_up_factor, 100000L,
             ^
/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:359:45: error: use of undeclared identifier 'FLAGS_jssp_scaling_up_factor'
          static_cast<int64>(round(weight * FLAGS_jssp_scaling_up_factor));
                                            ^
/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:375:19: error: use of undeclared identifier 'FLAGS_jssp_scaling_up_factor'
                  FLAGS_jssp_scaling_up_factor !=
                  ^
/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:385:17: error: use of undeclared identifier 'FLAGS_jssp_scaling_up_factor'
                FLAGS_jssp_scaling_up_factor);
                ^
/or-tools/or-tools/ortools/data/jobshop_scheduling_parser.cc:389:22: error: use of undeclared identifier 'FLAGS_jssp_scaling_up_factor'
              1.0L / FLAGS_jssp_scaling_up_factor);
                     ^

Edit: I did remove some patch in order to be the closest to what could be the source install by a user, I think that moving away from those patch (like some amelioration to the absl target name) would be easier to use system package.

@Mizux Mizux mentioned this pull request Mar 8, 2019
11 tasks
@Mizux
Copy link
Collaborator

Mizux commented Apr 12, 2019

Hi,

I revamped your PR in #1116 (commit a49b914) but with few changes:

  • Mostly Keep BUILD_dependencies options BUT

  • I don't use a Find*.cmake but simply call find_package(* REQUIRED) and let CMake to do its stuff

    • user can use CMAKE_PREFIX_PATH/CMAKE_MODULE_PATH if they want to hack/add hook etc..
  • all deps, if build, are installed in CMAKE_BINARY_DIR/dependencies/install/ so I just need to append this to CMAKE_PREFIX_PATH to see all available prebuilt dependencies.

  • I use a template https://github.com/google/or-tools/blob/master/cmake/dependencies/CMakeLists.txt.in for ExternalProject along a function in utils.cmake

    or-tools/cmake/utils.cmake

    Lines 66 to 126 in 4477092

    # build_git_dependency()
    #
    # CMake function to download, build and install (in staging area) a dependency at configure
    # time.
    #
    # Parameters:
    # NAME: name of the dependency
    # REPOSITORY: git url of the dependency
    # TAG: tag of the dependency
    # APPLY_PATCH: apply patch
    # CMAKE_ARGS: List of specific CMake args to add
    #
    # build_dependency(
    # NAME
    # abseil-cpp
    # URL
    # https://github.com/abseil/abseil-cpp.git
    # TAG
    # master
    # APPLY_PATCH
    # ${CMAKE_SOURCE_DIR}/patches/abseil-cpp.patch
    # )
    function(build_git_dependency)
    set(options "")
    set(oneValueArgs NAME REPOSITORY TAG APPLY_PATCH)
    set(multiValueArgs CMAKE_ARGS)
    cmake_parse_arguments(GIT_DEP
    "${options}"
    "${oneValueArgs}"
    "${multiValueArgs}"
    ${ARGN}
    )
    message(STATUS "Building ${GIT_DEP_NAME}: ...")
    if(GIT_DEP_APPLY_PATCH)
    set(PATCH_CMD "git apply \"${GIT_DEP_APPLY_PATCH}\"")
    else()
    set(PATCH_CMD "\"\"")
    endif()
    configure_file(
    ${CMAKE_CURRENT_SOURCE_DIR}/CMakeLists.txt.in
    ${CMAKE_CURRENT_BINARY_DIR}/${GIT_DEP_NAME}/CMakeLists.txt @ONLY)
    execute_process(
    COMMAND ${CMAKE_COMMAND} -H. -Bproject_build -G "${CMAKE_GENERATOR}"
    RESULT_VARIABLE result
    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${GIT_DEP_NAME})
    if(result)
    message(FATAL_ERROR "CMake step for ${GIT_DEP_NAME} failed: ${result}")
    endif()
    execute_process(
    COMMAND ${CMAKE_COMMAND} --build project_build --config ${CMAKE_BUILD_TYPE}
    RESULT_VARIABLE result
    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${GIT_DEP_NAME})
    if(result)
    message(FATAL_ERROR "Build step for ${GIT_DEP_NAME} failed: ${result}")
    endif()
    message(STATUS "Building ${GIT_DEP_NAME}: ...DONE")
    endfunction()

  • I use my new Cbc split repo forks of the official ones

  • I use the fixed abseil-cpp name i.e. absl::base instead of "broken" absl::absl_base since my internal path make its way to the public abseil-cpp repo.

Any feedback are welcome, hope you'll better understand what I'm aiming for....

@Mizux
Copy link
Collaborator

Mizux commented Apr 17, 2019

I'm closing this PR since we have #1116
Still open to any proposal, critics etc....

@Mizux Mizux closed this Apr 17, 2019
@Mizux Mizux self-assigned this Oct 26, 2020
@Mizux Mizux added this to the v9.0 milestone Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants